fix: resolve race condition in HookExecutionOrderTests by thomhurst · Pull Request #5355 · thomhurst/TUnit

thomhurst · 2026-04-03T12:07:07Z

Summary

GlobalHookExecutionOrderSetup.GlobalSetup has [BeforeEvery(Test)] which fires for every test in the assembly, not just HookExecutionOrderTests
When tests run concurrently, another test's BeforeEvery invocation calls Clear() on the shared _executionOrder list between the Before hook and the test body, causing the assertion to see count=2 instead of 3
Fix: filter the BeforeEvery hook to only act when the current test belongs to HookExecutionOrderTests

Test plan

HookExecutionOrderTests.VerifyExecutionOrder passes in isolation (both source-gen and reflection modes)
Full test suite passes with no flaky failures on this test

The BeforeEvery(Test) hook in GlobalHookExecutionOrderSetup fires for every test in the assembly. When tests run concurrently, another test's BeforeEvery invocation calls Clear() on the shared list between the Before hook and the test body, causing the test to see count=2 instead of 3. Filter the BeforeEvery hook to only act on HookExecutionOrderTests.

claude

Code Review

Fix is correct and the root cause diagnosis is accurate.

The hook firing for all tests in the assembly while clearing a shared static list is indeed the source of the race condition. The guard on properly contains the effect to the intended class.

Concern: Fragile static shared state

The fix resolves the cross-class interference, but _executionOrder is still a static List<string> — not thread-safe and still shared across any future tests added to HookExecutionOrderTests. The Clear() + Add("BeforeEvery") sequence is non-atomic, so if a second test method is ever added to this class and both run concurrently, the same race will resurface.

Suggested improvement — option A: Mark the class as non-parallel so it's always sequential:

[NotInParallel]
public class HookExecutionOrderTests

Suggested improvement — option B: Use per-test context storage instead of static state:

// In the global hook:
context.ObjectBag["executionOrder"] = new List<string>();
((List<string>)context.ObjectBag["executionOrder"]).Add("BeforeEvery");

// In Before(Test) and test body, read from context.ObjectBag

Option B is the more principled fix — it eliminates shared mutable state entirely and doesn't rely on filtering or parallelism attributes to prevent interference. It also scales naturally if more tests are added to the class.

That said, for a test-only file verifying hook ordering with a single test method, this PR is a pragmatic and correct fix for the immediate issue. The ClassType guard is idiomatic and readable. Happy to approve as-is or with the [NotInParallel] addition as a minimal safety net.

claude

Code Review

Fix is correct and the root cause diagnosis is accurate.

The [BeforeEvery(Test)] hook firing for all tests in the assembly while clearing a shared static list is indeed the source of the race condition. The guard on ClassType properly contains the effect to the intended class.

Concern: Fragile static shared state

The fix resolves the cross-class interference, but _executionOrder is still a static List<string> — not thread-safe and still shared across any future tests added to HookExecutionOrderTests. The Clear() + Add("BeforeEvery") sequence is non-atomic, so if a second test method is ever added to this class and both run concurrently, the same race will resurface.

Suggested improvement — option A: Mark the class as non-parallel so it's always sequential:

[NotInParallel]
public class HookExecutionOrderTests

Suggested improvement — option B: Use per-test context storage instead of static state:

// In the global hook:
context.ObjectBag["executionOrder"] = new List<string>();
((List<string>)context.ObjectBag["executionOrder"]).Add("BeforeEvery");

// In Before(Test) and test body, read from context.ObjectBag

Option B is the more principled fix — it eliminates shared mutable state entirely and doesn't rely on filtering or parallelism attributes to prevent interference. It also scales naturally if more tests are added to the class.

That said, for a test-only file verifying hook ordering with a single test method, this PR is a pragmatic and correct fix for the immediate issue. The ClassType guard is idiomatic and readable. Happy to approve as-is or with [NotInParallel] as a minimal safety net.

codacy-production · 2026-04-03T12:10:12Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric Results

Complexity 0

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

Addresses review feedback: the shared static List<string> is not thread-safe, so mark the class as non-parallel to prevent intra-class races if additional test methods are added in the future.

claude

Updated Code Review

The PR has been updated since the previous review and now incorporates both suggested improvements:

The ClassType guard in GlobalHookExecutionOrderSetup.GlobalSetup — prevents cross-class interference from the [BeforeEvery(Test)] hook.
[NotInParallel] on HookExecutionOrderTests — prevents intra-class races between multiple test methods.

Both layers of protection are in place. The fix is correct, minimal, and addresses the root cause accurately.

Remaining minor observation

The static _executionOrder list is still shared mutable state, which means if HookExecutionOrderTests ever gains a second test method, [NotInParallel] will serialize execution and prevent races — but the Clear() call in the global hook still mutates state owned by another class, which is an unusual coupling. This is not a blocker for this PR; it is a design smell worth being aware of if the test class grows.

Verdict

The fix is pragmatic, correct, and the added [NotInParallel] provides the safety net called out in the previous review. No blocking issues. Ready to merge.

Replace the shared static List<string> with per-test context StateBag storage. Each test gets its own isolated list, eliminating the race condition at the root rather than working around it with [NotInParallel] and type guards. The type guard in the BeforeEvery hook is still needed (it fires for all tests) but no longer protects shared mutable state.

claude

Code Review — PR #5355: fix: resolve race condition in HookExecutionOrderTests

Summary of changes

The PR fixes a genuine flakiness bug in HookExecutionOrderTests. The original code used a static readonly List<string> _executionOrder field shared across all tests in the assembly. A [BeforeEvery(Test)] hook called Clear() on that list before every test — not just tests belonging to HookExecutionOrderTests. Under parallel execution, another test's BeforeEvery invocation could clear the list between the Before(Test) hook and the test body of VerifyExecutionOrder, causing an incorrect count.

The final state of the PR (commit cfb2fa80) takes the principled fix: it replaces the shared static list entirely with per-test StateBag storage, and adds a class-type guard in the global hook so it only acts on HookExecutionOrderTests tests.

What works well

Root cause is correctly diagnosed. The race was real and the described failure mode is accurate. [BeforeEvery(Test)] is assembly-scoped; calling Clear() on a static collection inside it is inherently unsafe when other tests run concurrently.

StateBag is the right abstraction here. TestContext.StateBag is per-test-context, backed by a ConcurrentDictionary<string, object?>. Using it eliminates all shared mutable state. Each test invocation gets its own List<string> initialized by GetOrAdd, so no cross-test interference is possible regardless of parallelism. This is the idiomatic TUnit approach for passing data between lifecycle hooks and test bodies.

The class-type guard is a correct secondary defence. Even with StateBag, having the global BeforeEvery hook silently add entries to the StateBag of every test in the assembly would be unexpected and wasteful. Filtering on ClassType is the right call.

Dropping [NotInParallel] (added in an intermediate commit, removed in the final one) is also the right call. The StateBag approach makes serialization unnecessary since there is no shared state left.

Suggestions

1. `GetOrAdd` in all hooks obscures ownership semantics

var order = context.StateBag.GetOrAdd<List<string>>("executionOrder", _ => []);
order.Add("BeforeEvery");

GetOrAdd is "get if present, create if absent." In GlobalSetup (the [BeforeEvery(Test)] hook), the list will never already exist — the StateBag is fresh per test context when the global hook fires first. Using it in all three sites masks which hook owns initialization vs. which hooks only read.

A more explicit pattern makes the ownership clear and turns a missing initialization into an immediate failure rather than a subtly wrong count:

// GlobalSetup: explicitly owns initialization
context.StateBag["executionOrder"] = new List<string> { "BeforeEvery" };

// InstanceSetup and test body: retrieval only
var order = (List<string>)TestContext.Current!.StateBag["executionOrder"]!;
order.Add("Before");

This is a readability improvement, not a correctness fix — the existing count assertion would catch a missing initialization either way.

2. Cross-class coupling: `GlobalHookExecutionOrderSetup` now depends on `HookExecutionOrderTests`

if (context.Metadata.TestDetails.ClassType != typeof(HookExecutionOrderTests))
    return;

GlobalHookExecutionOrderSetup has a compile-time reference to a sibling test class. For a test-only file this is perfectly acceptable, but it means renaming or moving HookExecutionOrderTests silently breaks the guard (renamed type → ClassType never matches → list is never populated → order.Count.ShouldBe(3) fails). A comment explaining the guard and the coupling would help future contributors.

An alternative is to move the [BeforeEvery(Test)] hook inside HookExecutionOrderTests as a nested setup class, collocating the concern:

public class HookExecutionOrderTests
{
    public sealed class Setup
    {
        [BeforeEvery(Test)]
        public static void GlobalSetup(TestContext context)
        {
            if (context.Metadata.TestDetails.ClassType != typeof(HookExecutionOrderTests)) return;
            // ...
        }
    }
}

This would need a quick check that TUnit's source generator picks up [BeforeEvery(Test)] on nested classes, but it would remove the cross-file coupling entirely.

Verdict

The fix is correct and well-reasoned. The StateBag approach is strictly better than the original static list design. No blocking issues — the two points above are readability/maintainability suggestions, not correctness concerns.

Ready to merge.

Updated [TUnit.Core](https://github.com/thomhurst/TUnit) from 1.23.7 to 1.28.7. <details> <summary>Release notes</summary> _Sourced from [TUnit.Core's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.28.7  ## What's Changed ### Other Changes * fix: prevent StringBuilder race in console interceptor during parallel tests by @thomhurst in thomhurst/TUnit#5414 ### Dependencies * chore(deps): update tunit to 1.28.5 by @thomhurst in thomhurst/TUnit#5415 **Full Changelog**: thomhurst/TUnit@v1.28.5...v1.28.7 ## 1.28.5  ## What's Changed ### Other Changes * perf: eliminate redundant builds in CI pipeline by @thomhurst in thomhurst/TUnit#5405 * perf: eliminate store.ToArray() allocation on mock behavior execution hot path by @thomhurst in thomhurst/TUnit#5409 * fix: omit non-class/struct constraints on explicit interface mock implementations by @thomhurst in thomhurst/TUnit#5413 ### Dependencies * chore(deps): update tunit to 1.28.0 by @thomhurst in thomhurst/TUnit#5406 **Full Changelog**: thomhurst/TUnit@v1.28.0...v1.28.5 ## 1.28.0  ## What's Changed ### Other Changes * fix: resolve build warnings in solution by @thomhurst in thomhurst/TUnit#5386 * Perf: Optimize MockEngine hot paths (~30-42% faster) by @thomhurst in thomhurst/TUnit#5391 * Move Playwright install into pipeline module by @thomhurst in thomhurst/TUnit#5390 * perf: optimize solution build performance by @thomhurst in thomhurst/TUnit#5393 * perf: defer per-class JIT via lazy test registration + parallel resolution by @thomhurst in thomhurst/TUnit#5395 * Perf: Generate typed HandleCall<T1,...> overloads to eliminate argument boxing by @thomhurst in thomhurst/TUnit#5399 * perf: filter generated attributes to TUnit-related types only by @thomhurst in thomhurst/TUnit#5402 * fix: generate valid mock class names for generic interfaces with non-built-in type args by @thomhurst in thomhurst/TUnit#5404 ### Dependencies * chore(deps): update tunit to 1.27.0 by @thomhurst in thomhurst/TUnit#5392 * chore(deps): update dependency path-to-regexp to v8 by @thomhurst in thomhurst/TUnit#5378 **Full Changelog**: thomhurst/TUnit@v1.27.0...v1.28.0 ## 1.27.0  ## What's Changed ### Other Changes * Fix Dependabot security vulnerabilities in docs site by @thomhurst in thomhurst/TUnit#5372 * fix: use 0.0.0-scrubbed sentinel version in snapshot scrubber to avoid false Dependabot alerts by @thomhurst in thomhurst/TUnit#5374 * Speed up Engine.Tests by removing ProcessorCount parallelism cap by @thomhurst in thomhurst/TUnit#5379 * ci: add concurrency groups to cancel redundant workflow runs by @thomhurst in thomhurst/TUnit#5373 * Add scope-aware initialization and disposal OpenTelemetry spans to trace timeline and HTML report by @Copilot in thomhurst/TUnit#5339 * Add WithInnerExceptions() for fluent AggregateException assertion chaining by @thomhurst in thomhurst/TUnit#5380 * Drop net6.0 and net7.0 TFMs, keep net8.0+ and netstandard2.x by @thomhurst in thomhurst/TUnit#5387 * Remove all [Obsolete] members and migrate callers by @thomhurst in thomhurst/TUnit#5384 * Add AssertionResult.Failed overload that accepts an Exception by @thomhurst in thomhurst/TUnit#5388 ### Dependencies * chore(deps): update dependency mockolate to 2.3.0 by @thomhurst in thomhurst/TUnit#5370 * chore(deps): update tunit to 1.25.0 by @thomhurst in thomhurst/TUnit#5371 * chore(deps): update dependency minimatch to v9.0.9 by @thomhurst in thomhurst/TUnit#5375 * chore(deps): update dependency path-to-regexp to v0.2.5 by @thomhurst in thomhurst/TUnit#5376 * chore(deps): update dependency minimatch to v10 by @thomhurst in thomhurst/TUnit#5377 * chore(deps): update dependency picomatch to v4 by @thomhurst in thomhurst/TUnit#5382 * chore(deps): update dependency svgo to v4 by @thomhurst in thomhurst/TUnit#5383 * chore(deps): update dependency path-to-regexp to v1 [security] by @thomhurst in thomhurst/TUnit#5385 **Full Changelog**: thomhurst/TUnit@v1.25.0...v1.27.0 ## 1.25.0  ## What's Changed ### Other Changes * Fix missing `default` constraint on explicit interface implementations with unconstrained generics by @thomhurst in thomhurst/TUnit#5363 * feat(mocks): add ReturnsAsync typed factory overload with method parameters by @thomhurst in thomhurst/TUnit#5367 * Fix Arg.IsNull<T> and Arg.IsNotNull<T> to support nullable value types by @thomhurst in thomhurst/TUnit#5366 * refactor(mocks): use file-scoped types for generated implementation details by @thomhurst in thomhurst/TUnit#5369 * Compress HTML report JSON data and minify CSS by @thomhurst in thomhurst/TUnit#5368 ### Dependencies * chore(deps): update tunit to 1.24.31 by @thomhurst in thomhurst/TUnit#5356 * chore(deps): update dependency mockolate to 2.2.0 by @thomhurst in thomhurst/TUnit#5357 * chore(deps): update dependency polyfill to 9.24.1 by @thomhurst in thomhurst/TUnit#5365 * chore(deps): update dependency polyfill to 9.24.1 by @thomhurst in thomhurst/TUnit#5364 **Full Changelog**: thomhurst/TUnit@v1.24.31...v1.25.0 ## 1.24.31  ## What's Changed ### Other Changes * Fix Aspire 13.2.0+ timeout caused by ProjectRebuilderResource being awaited by @Copilot in thomhurst/TUnit#5335 * chore(deps): update dependency polyfill to 9.24.0 by @thomhurst in thomhurst/TUnit#5349 * Fix nullable IParsable type recognition in source generator and analyzer by @Copilot in thomhurst/TUnit#5354 * fix: resolve race condition in HookExecutionOrderTests by @thomhurst in thomhurst/TUnit#5355 * Fix MaxExternalSpansPerTest cap bypass when Activity.Parent chain is broken by @Copilot in thomhurst/TUnit#5352 ### Dependencies * chore(deps): update tunit to 1.24.18 by @thomhurst in thomhurst/TUnit#5340 * chore(deps): update dependency stackexchange.redis to 2.12.14 by @thomhurst in thomhurst/TUnit#5343 * chore(deps): update verify to 31.15.0 by @thomhurst in thomhurst/TUnit#5346 * chore(deps): update dependency polyfill to 9.24.0 by @thomhurst in thomhurst/TUnit#5348 **Full Changelog**: thomhurst/TUnit@v1.24.18...v1.24.31 ## 1.24.18  ## What's Changed ### Other Changes * feat(mocks): shorter, more readable generated mock type names by @thomhurst in thomhurst/TUnit#5334 * Fix DisposeAsync() ordering for nested property injection by @Copilot in thomhurst/TUnit#5337 ### Dependencies * chore(deps): update tunit to 1.24.13 by @thomhurst in thomhurst/TUnit#5331 **Full Changelog**: thomhurst/TUnit@v1.24.13...v1.24.18 ## 1.24.13  ## What's Changed ### Other Changes * perf(mocks): optimize MockEngine for lower allocation and faster verification by @thomhurst in thomhurst/TUnit#5319 * Remove defunct `UseTestingPlatformProtocol` reference for vscode by @erwinkramer in thomhurst/TUnit#5328 * perf(aspnetcore): prevent thread pool starvation during parallel WebApplicationTest server init by @thomhurst in thomhurst/TUnit#5329 * fix TUnit0073 for when type from from another assembly by @SimonCropp in thomhurst/TUnit#5322 * Fix implicit conversion operators bypassed in property injection casts by @Copilot in thomhurst/TUnit#5317 * fix(mocks): skip non-virtual 'new' methods when discovering mockable members by @thomhurst in thomhurst/TUnit#5330 * feat(mocks): IFoo.Mock() discovery with generic fallback and ORP resolution by @thomhurst in thomhurst/TUnit#5327 ### Dependencies * chore(deps): update tunit to 1.24.0 by @thomhurst in thomhurst/TUnit#5315 * chore(deps): update aspire to 13.2.1 by @thomhurst in thomhurst/TUnit#5323 * chore(deps): update verify to 31.14.0 by @thomhurst in thomhurst/TUnit#5325 ## New Contributors * @erwinkramer made their first contribution in thomhurst/TUnit#5328 **Full Changelog**: thomhurst/TUnit@v1.24.0...v1.24.13 ## 1.24.0  ## What's Changed ### Other Changes * perf: optimize TUnit.Mocks hot paths by @thomhurst in thomhurst/TUnit#5304 * fix: resolve System.Memory version conflict on .NET Framework (net462) by @thomhurst in thomhurst/TUnit#5303 * fix: resolve CS0460/CS0122/CS0115 when mocking concrete classes from external assemblies by @thomhurst in thomhurst/TUnit#5310 * feat(mocks): parameterless Returns() and ReturnsAsync() for async methods by @thomhurst in thomhurst/TUnit#5309 * Fix typo in NUnit manual migration guide by @aa-ko in thomhurst/TUnit#5312 * refactor(mocks): unify Mock.Of<T>() and Mock.OfPartial<T>() into single API by @thomhurst in thomhurst/TUnit#5311 * refactor(mocks): clean up Mock API surface by @thomhurst in thomhurst/TUnit#5314 * refactor(mocks): remove generic/untyped overloads from public API by @thomhurst in thomhurst/TUnit#5313 ### Dependencies * chore(deps): update tunit to 1.23.7 by @thomhurst in thomhurst/TUnit#5305 * chore(deps): update dependency mockolate to 2.1.1 by @thomhurst in thomhurst/TUnit#5307 ## New Contributors * @aa-ko made their first contribution in thomhurst/TUnit#5312 **Full Changelog**: thomhurst/TUnit@v1.23.7...v1.24.0 Commits viewable in [compare view](thomhurst/TUnit@v1.23.7...v1.28.7). </details> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=TUnit.Core&package-manager=nuget&previous-version=1.23.7&new-version=1.28.7)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

thomhurst temporarily deployed to Pull Requests April 3, 2026 12:07 — with GitHub Actions Inactive

claude bot reviewed Apr 3, 2026

View reviewed changes

add [NotInParallel] to HookExecutionOrderTests

2931368

Addresses review feedback: the shared static List<string> is not thread-safe, so mark the class as non-parallel to prevent intra-class races if additional test methods are added in the future.

thomhurst temporarily deployed to Pull Requests April 3, 2026 12:10 — with GitHub Actions Inactive

claude bot reviewed Apr 3, 2026

View reviewed changes

thomhurst temporarily deployed to Pull Requests April 3, 2026 12:12 — with GitHub Actions Inactive

thomhurst enabled auto-merge (squash) April 3, 2026 12:13

claude bot reviewed Apr 3, 2026

View reviewed changes

thomhurst merged commit 36a930a into main Apr 3, 2026
15 checks passed

thomhurst deleted the fix/hook-execution-order-test-race-condition branch April 3, 2026 12:50

dependabot bot mentioned this pull request Apr 3, 2026

Bump TUnit from 1.23.7 to 1.24.31 SimonCropp/NaughtyStrings#610

Closed

claude bot mentioned this pull request Apr 3, 2026

chore(deps): update tunit to 1.24.31 #5356

Merged

1 task

This was referenced Apr 6, 2026

Bump TUnit from 1.24.13 to 1.28.7 MarcelMichau/dotnet-starter-project-template#138

Open

Chore(deps): Bump TUnit from 1.24.0 to 1.28.7 code-of-chaos/cs-code_of_chaos-cli_args_parser#74

Merged

Bump TUnit from 1.21.6 to 1.28.7 Dippere/Telegraph.Sharp#112

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: resolve race condition in HookExecutionOrderTests#5355

fix: resolve race condition in HookExecutionOrderTests#5355
thomhurst merged 3 commits intomainfrom
fix/hook-execution-order-test-race-condition

thomhurst commented Apr 3, 2026

Uh oh!

claude bot left a comment

Uh oh!

claude bot left a comment

Uh oh!

codacy-production bot commented Apr 3, 2026 •

edited

Loading

Uh oh!

claude bot left a comment

Uh oh!

claude bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

thomhurst commented Apr 3, 2026

Summary

Test plan

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Code Review

Concern: Fragile static shared state

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Code Review

Concern: Fragile static shared state

Uh oh!

codacy-production bot commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Up to standards ✅

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Updated Code Review

Remaining minor observation

Verdict

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Code Review — PR #5355: fix: resolve race condition in HookExecutionOrderTests

Summary of changes

What works well

Suggestions

1. GetOrAdd in all hooks obscures ownership semantics

2. Cross-class coupling: GlobalHookExecutionOrderSetup now depends on HookExecutionOrderTests

Verdict

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

codacy-production bot commented Apr 3, 2026 •

edited

Loading

1. `GetOrAdd` in all hooks obscures ownership semantics

2. Cross-class coupling: `GlobalHookExecutionOrderSetup` now depends on `HookExecutionOrderTests`